Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganize shootout benchmarks into a single directory #260

Merged
merged 10 commits into from
Jul 7, 2023

Conversation

abrown
Copy link
Collaborator

@abrown abrown commented Jul 6, 2023

Now that #251 and #256 make it possible for more than one benchmark to live in a single directory, this change moves all of the shootout artifacts into a single directory. The commits in this PR perform these steps atomically and can be reviewed sequentially. Even with the documentation I added in several places, this supposes a large decrease in the number of lines of code to maintain; the main benefit here is likely coming from using a single script to build all of the native benchmarks. Upon recompilation, the WebAssembly benchmarks have increased in size and I'm not exactly sure why.

abrown added 9 commits June 29, 2023 17:29
Now that bytecodealliance#251 and bytecodealliance#256 make it possible for more than one benchmark to
live in a single directory, this change moves all of the shootout
artifacts into a single directory. This simply performs the file
movement; subsequent commits will make necessary tweaks.
This change refactors how the shootout native benchmarks are built. The
`Dockerfile.native` file is retained and is expected to be _the_ way to
build the native shared libraries for this kind of benchmarking. A
`build-native.sh` script is included in the directory to (a) be used by
`Dockerfile.native` and (b) for building the native benchmarks in
environments where running Docker may not be possible.

Now that all of the benchmarks are built in one directory, the native
libraries cannot all be named `benchmark.so`. Because of this and the
hard-coded path expected by the native engine (see bytecodealliance#259), this change
also modifies the associated `*-native.sh` scripts to set up a temporary
directory that looks like the `benchmark.so` environment that was there
previously. This additional logic could be removed once bytecodealliance#259 is fixed.
These are all migrated over to be a part of the single `shootout`
directory.
The new file structure for `shootout` now expects these paths to look
like `shootout-ackermann.*.input`.
When we allocate the array to sort, we should do so with items of size
`double` (64 bits) instead of `double*` (32 bits in WebAssembly). I am
very confused as to why this benchmark worked previously, but when I
recompiled it prior to this change, it would invariably fail due to
accessing addresses beyond the memory bounds.
This change fixes some issues highlighted by CI:
- it adds more verbose output to see which commands are executed
- it improves the documentation to clarify how to use certain flags
- it fixes slight mistakes in the scripts missed by previous refactoring
- and, __most especially__, it alters the order of the parameters passed
  to compile the native libraries.

This last change is indicative of the fragility of the native
benchmarks: apparently moving `-lengine` to the end was necessary for
the linker to understand which library provides `bench_start` and
`bench_end`.
@abrown abrown marked this pull request as ready for review July 6, 2023 04:01
@abrown abrown requested review from jlb6740 and fitzgen July 6, 2023 04:01
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't look in detail, but moving them all to a shared directory makes sense to me.

Copy link
Collaborator

@jlb6740 jlb6740 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you run native benchmarks for shootout now? The READMEs need to be updated. From what I can tell this refactoring eliminates the cargo build system used for native that was just put in place?

@jlb6740
Copy link
Collaborator

jlb6740 commented Jul 6, 2023

How do you run native benchmarks for shootout now? The READMEs need to be updated. From what I can tell this refactoring eliminates the cargo build system used for native that was just put in place?

Note, when I tried native does build and seem to execute fine .. it's just are we ok with doing away with the Cargo build system and the update of the READMEs (in the native engine folder and the benchmark folder) where the former's instructions on how to build are now wrong and the later explicitly tells developers that the benchmark folder is required to supply a cargo based build if they want to support a native build.

Now that `Dockerfile.native` relies on a script, `build-native.sh`,
instead of the Cargo build system, the documentation for building native
libraries has to change.
Copy link
Collaborator

@jlb6740 jlb6740 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Looks good to me.

@abrown abrown merged commit 76769a5 into bytecodealliance:main Jul 7, 2023
@abrown abrown deleted the reorganize-shootout branch July 7, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants